Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #38029 - Update subscription_manager_setup to use correct URLs for load-balanced smart proxies #10382

Merged

Conversation

jeremylenz
Copy link
Contributor

@jeremylenz jeremylenz commented Nov 20, 2024

The subscription_manager_setup snippet, when used in provisioning, was setting the wrong URLs for subscription-manager for load-balanced smart proxy setups. This attempts to fix this by using registration_url, which should correctly reflect the load balancer FQDN when the smart proxy is set up correctly with load balancing, and will reflect the smart proxy FQDN when it's not.

Goes with Katello/katello#11229

@@ -88,11 +88,11 @@ fi
end
elsif @subman_setup_scenario == 'provisioning'
if plugin_present?('katello')
server_hostname = @host.content_source.rhsm_url.host
server_hostname = @host.content_source.registration_url.host
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using registration_url.host here ensures that it uses the hostname of the load balancer, not of the underlying smart proxy.

server_port = @host.content_source.rhsm_url.port
server_prefix = @host.content_source.rhsm_url.path
repo_ca_cert = "$KATELLO_SERVER_CA_CERT"
rhsm_baseurl = @host.content_source.pulp_content_url
rhsm_baseurl = @host.content_source.load_balancer_pulp_content_url
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added load_balancer_pulp_content_url which is the same as pulp_content_url except it uses registration_url as the base instead of setting(SmartProxy::PULP3_FEATURE, 'content_app_url'). Hopefully that doesn't break anything.

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes make sense to me. I want to clarify: With this, don't we have to make any UI changes, as was discussed in the Slack thread?

@jeremylenz
Copy link
Contributor Author

I want to clarify: With this, don't we have to make any UI changes, as was discussed in the Slack thread?

Correct, this will resolve the issue without any UI changes. The host "registered through" will be correct again, so Hammer will display the correct "Registered To" value.

However, the "Registration Details" card in the UI can be additionally improved - I kinda forgot since I was distracted by getting to the root of the issue :) I can update the UI card to display load balancer status and "registered through" value. I can do it either in the existing Katello PR or in a separate one. thoughts?

@stejskalleos
Copy link
Contributor

I can do it either in the existing Katello PR or in a separate one. thoughts?

If the effort is small, I vote to do it in one PR.

@jeremylenz
Copy link
Contributor Author

jeremylenz commented Nov 22, 2024

If the effort is small, I vote to do it in one PR.

Updated Katello/katello#11229 👍

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code change looks good to me; let's wait for QE ACK

@shubhamsg199
Copy link

Couldn't apply the patch due to some conflicts, but tested by adding the changes manually.
Provisioning worked successfully and the host was registered through the loadbalancer FQDN

@stejskalleos stejskalleos merged commit afc6ac3 into theforeman:develop Nov 29, 2024
43 of 52 checks passed
@stejskalleos
Copy link
Contributor

Thanks @jeremylenz @shubhamsg199

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants